Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WIP] Visibility trigger for non-amp elements #26995

Closed
wants to merge 7 commits into from
Closed

[WIP] Visibility trigger for non-amp elements #26995

wants to merge 7 commits into from

Conversation

micajuine-ho
Copy link
Contributor

To be reviewed/merged after #26823's changes. Guarded with the same feature flag

For #20607

Within the IntersectionObserverPolyfill, if the element is a non-amp element (no getLayoutBox, no owner), asynchronously use getBoundingClientRect (offset by the hostViewport) to find the element's rect.

Besides affecting VisibilityManager, this change also impacts RefreshManager (for a4a). Since it manages a4a, we're guaranteed to always get an amp-element so this should not change behavior for it.

@micajuine-ho micajuine-ho removed the request for review from dvoytenko February 26, 2020 22:44
jridgewell
jridgewell reviewed Mar 2, 2020
View reviewed changes
extensions/amp-analytics/0.1/analytics-root.js Outdated

// DOM search can "look" outside the boundaries of the root, thus make
// sure the result is contained. Length is not supported in all browsers
if (foundElements && foundElements.length) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not necessary, querySelector is already guaranteed to contain everything that is returned.

What you're probably thinking of is that querySelector('div img') can match on <div><root><img/></root></div>, but this contains check isn't going to fix that. Use scopedQuerySelector from src/dom.js

src/layout-rect.js Show resolved Hide resolved
@mrjoro
Copy link
Member

mrjoro commented Jun 12, 2020

Is this PR still active?

@micajuine-ho
Copy link
Contributor Author

micajuine-ho commented Jun 12, 2020 •
edited
Loading

Yes, we are waiting for Intersection Observer experiment to be out for a while, then we will proceed with this PR.

Edit: Will pick up this work after TCFv2 changes, (end of June)

@dvoytenko
Copy link
Contributor

Just an update: intersection observer experiment is fully launched. Just cleanup is remaining. It can be now used throughout.

Micajuine Ho added 2 commits July 7, 2020 11:02
@amp-owners-bot
Copy link

amp-owners-bot bot commented Jul 7, 2020 •
edited by ampprojectbot
Loading

Hey @rsimha! These files were changed:

.vscode/settings.json
build-system/compile/closure-compile.js
build-system/compile/compile.js
build-system/pr-check/OWNERS
build-system/pr-check/build-targets.js
build-system/pr-check/build.js
build-system/pr-check/checks.js
build-system/pr-check/cross-browser-tests.js
build-system/pr-check/dist-bundle-size.js
build-system/pr-check/dist-tests.js
build-system/pr-check/e2e-tests.js
build-system/pr-check/experiment-tests.js
+18 more

Hey @erwinmombay! These files were changed:

build-system/babel-plugins/babel-plugin-amp-mode-transformer/index.js
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/no-transform/input.js
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/no-transform/options.json
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/no-transform/output.js
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/nomodule/input.js
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/nomodule/options.json
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/nomodule/output.mjs
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/transform/input.js
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/transform/options.json
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/transform/output.mjs
build-system/babel-plugins/babel-plugin-const-transformer/index.js
build-system/babel-plugins/babel-plugin-const-transformer/test/fixtures/transform-assertions/ignored-let/options.json
+295 more

Hey @jridgewell! These files were changed:

build-system/babel-plugins/babel-plugin-amp-mode-transformer/index.js
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/no-transform/input.js
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/no-transform/options.json
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/no-transform/output.js
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/nomodule/input.js
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/nomodule/options.json
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/nomodule/output.mjs
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/transform/input.js
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/transform/options.json
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/transform/output.mjs
build-system/babel-plugins/babel-plugin-const-transformer/index.js
build-system/babel-plugins/babel-plugin-const-transformer/test/fixtures/transform-assertions/ignored-let/options.json
+290 more

Hey @danielrozenberg! These files were changed:

build-system/compile/internal-version.js
build-system/global-configs/experiments-config.json
build-system/sauce_connect/OWNERS
build-system/sauce_connect/start_sauce_connect.sh
build-system/sauce_connect/stop_sauce_connect.sh
build-system/tasks/visual-diff/.percy.yaml
build-system/tasks/visual-diff/helpers.js
build-system/tasks/visual-diff/index.js
build-system/tasks/visual-diff/package.json
build-system/tasks/visual-diff/yarn.lock

Hey @alanorozco! These files were changed:

build-system/server/app-index/amphtml-helpers.js
build-system/server/app-index/api/api.js
build-system/server/app-index/file-list.js
build-system/server/app-index/form.js
build-system/server/app-index/html.js
build-system/server/app-index/template.js
build-system/server/app-index/url.js
build-system/server/app-index/util/listing.js
extensions/amp-3q-player/0.1/amp-3q-player.js
extensions/amp-3q-player/0.1/test/test-amp-3q-player.js
extensions/amp-3q-player/amp-3q-player.md
extensions/amp-brid-player/0.1/amp-brid-player.js
+39 more

Hey @estherkim! These files were changed:

build-system/tasks/e2e/README.md
build-system/tasks/e2e/amp-driver.js
build-system/tasks/e2e/controller-promise.js
build-system/tasks/e2e/describes-e2e.js
build-system/tasks/e2e/driver/query-xpath.js
build-system/tasks/e2e/expect.js
build-system/tasks/e2e/functional-test-controller.js
build-system/tasks/e2e/index.js
build-system/tasks/e2e/network-logger.js
build-system/tasks/e2e/package.json
build-system/tasks/e2e/puppeteer-controller.js
build-system/tasks/e2e/repl.js
+2 more

Hey @wassgha! These files were changed:

build-system/tasks/storybook/OWNERS
build-system/tasks/storybook/amp-env/decorator.js
build-system/tasks/storybook/amp-env/main.js
build-system/tasks/storybook/amp-env/register.js
build-system/tasks/storybook/amp-env/webpack.config.js
build-system/tasks/storybook/index.js
build-system/tasks/storybook/package.json
build-system/tasks/storybook/preact-env/main.js
build-system/tasks/storybook/preact-env/webpack.config.js
build-system/tasks/storybook/yarn.lock
extensions/amp-3q-player/0.1/amp-3q-player.js
extensions/amp-3q-player/0.1/test/test-amp-3q-player.js
+42 more

Hey @Jiaming-X! These files were changed:

extensions/amp-ad-network-adsense-impl/0.1/adsense-shared-state.js
extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js
extensions/amp-ad-network-adsense-impl/0.1/responsive-state.js
extensions/amp-ad-network-adsense-impl/0.1/test/test-amp-ad-network-adsense-impl.js
extensions/amp-ad-network-adsense-impl/0.1/test/test-responsive-state.js
extensions/amp-ad-network-adsense-impl/OWNERS

Hey @jeffkaufman! These files were changed:

extensions/amp-ad-network-adsense-impl/0.1/adsense-shared-state.js
extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js
extensions/amp-ad-network-adsense-impl/0.1/responsive-state.js
extensions/amp-ad-network-adsense-impl/0.1/test/test-amp-ad-network-adsense-impl.js
extensions/amp-ad-network-adsense-impl/0.1/test/test-responsive-state.js
extensions/amp-ad-network-adsense-impl/OWNERS
extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js
extensions/amp-ad-network-doubleclick-impl/0.1/flexible-ad-slot-utils.js
extensions/amp-ad-network-doubleclick-impl/0.1/safeframe-host.js
extensions/amp-ad-network-doubleclick-impl/0.1/sra-utils.js
extensions/amp-ad-network-doubleclick-impl/0.1/test/test-amp-ad-network-doubleclick-impl.js
extensions/amp-ad-network-doubleclick-impl/0.1/test/test-doubleclick-fluid.js
+8 more

Hey @gmajoulet! These files were changed:

extensions/amp-story-360/0.1/amp-story-360.css
extensions/amp-story-360/0.1/amp-story-360.js
extensions/amp-story-360/0.1/test/test-amp-story-360.js
extensions/amp-story-360/OWNERS
extensions/amp-story-education/0.1/amp-story-education.css
extensions/amp-story-education/0.1/amp-story-education.js
extensions/amp-story-education/0.1/test/test-amp-story-education.js
extensions/amp-story-education/OWNERS
extensions/amp-story-education/amp-story-education.md
extensions/amp-story-education/validator-amp-story-education.protoascii
extensions/amp-story/0.1/_locales/default.js
extensions/amp-story/0.1/_locales/en.js
+242 more

Hey @processprocess, @t0mg! These files were changed:

extensions/amp-story-360/0.1/amp-story-360.css
extensions/amp-story-360/0.1/amp-story-360.js
extensions/amp-story-360/0.1/test/test-amp-story-360.js
extensions/amp-story-360/OWNERS

Hey @newmuis! These files were changed:

extensions/amp-story/0.1/_locales/default.js
extensions/amp-story/0.1/_locales/en.js
extensions/amp-story/0.1/amp-story-base-layer.js
extensions/amp-story/0.1/amp-story-bookend.css
extensions/amp-story/0.1/amp-story-bookend.js
extensions/amp-story/0.1/amp-story-consent.css
extensions/amp-story/0.1/amp-story-consent.js
extensions/amp-story/0.1/amp-story-cta-layer.js
extensions/amp-story/0.1/amp-story-desktop-user-overridable.css
extensions/amp-story/0.1/amp-story-desktop.css
extensions/amp-story/0.1/amp-story-grid-layer.js
extensions/amp-story/0.1/amp-story-hint.css
+232 more

Hey @Enriqe! These files were changed:

extensions/amp-story/0.1/_locales/default.js
extensions/amp-story/0.1/_locales/en.js
extensions/amp-story/0.1/amp-story-base-layer.js
extensions/amp-story/0.1/amp-story-bookend.css
extensions/amp-story/0.1/amp-story-bookend.js
extensions/amp-story/0.1/amp-story-consent.css
extensions/amp-story/0.1/amp-story-consent.js
extensions/amp-story/0.1/amp-story-cta-layer.js
extensions/amp-story/0.1/amp-story-desktop-user-overridable.css
extensions/amp-story/0.1/amp-story-desktop.css
extensions/amp-story/0.1/amp-story-grid-layer.js
extensions/amp-story/0.1/amp-story-hint.css
+239 more

Hey @ampproject/wg-caching! These files were changed:

validator/AUTHORS
validator/BUILD
validator/README.md
validator/WORKSPACE
validator/build.py
validator/engine/amp4ads-parse-css.js
validator/engine/amp4ads-parse-css_test.js
validator/engine/htmlparser-interface.js
validator/engine/htmlparser.js
validator/engine/htmlparser_test.js
validator/engine/json-testutil.js
validator/engine/keyframes-parse-css.js
+235 more

@micajuine-ho micajuine-ho changed the title Visibility trigger for non-amp elements [WIP] Visibility trigger for non-amp elements Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jridgewell jridgewell jridgewell left review comments

@zhouyx zhouyx Awaiting requested review from zhouyx

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@micajuine-ho @mrjoro @dvoytenko @jridgewell @googlebot